Neutral Atom hybrid compilation extension#1293
Conversation
# Conflicts: # include/mqt-core/QuantumComputation.hpp
… as floating point
📝 WalkthroughWalkthroughAdds a Bridge operation type, a QuantumComputation::bridge(const Targets&) API, OpenQASM emission for "bridge", refactors AodOperation OpenQASM serialization and adds per-dimension helpers, updates op-type enumeration, and adds/adjusts tests for Bridge and AodMove output. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant QC as QuantumComputation
participant Op as StandardOperation
participant QASM as OpenQASM
User->>QC: bridge(targets)
activate QC
QC->>QC: validate target indices
QC->>Op: create StandardOperation(type=Bridge, targets)
QC->>QC: append operation to circuit
deactivate QC
Note right of Op `#DDEFEF`: serialization later
Op->>Op: dumpGateType() -> "bridge"
Op->>QASM: emit "bridge q[i], q[j], ..."
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)src/ir/operations/AodOperation.cpp (5)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
burgholzer
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/ir/operations/AodOperation.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ir/operations/AodOperation.cpp (4)
include/mqt-core/ir/operations/AodOperation.hpp (9)
dir(77-77)dir(79-79)dir(81-81)dir(83-83)of(85-88)maybe_unused(69-69)maybe_unused(69-69)maybe_unused(71-71)maybe_unused(71-71)src/ir/operations/StandardOperation.cpp (2)
dumpOpenQASM(276-290)dumpOpenQASM(276-279)src/ir/QuantumComputation.cpp (2)
dumpOpenQASM(634-703)dumpOpenQASM(634-634)src/ir/operations/CompoundOperation.cpp (2)
dumpOpenQASM(191-199)dumpOpenQASM(191-195)
🔇 Additional comments (4)
src/ir/operations/AodOperation.cpp (4)
45-52: LGTM: Helper method implementation is correct.The
convertToDimensionhelper correctly converts the raw direction vector to theDimensionenum type.
106-132: LGTM: New dimension helper methods are well-implemented.The three new helper methods (
getStarts,getMaxDistance,getDistances) follow a consistent pattern with the existinggetEndsmethod and correctly filter/compute per-dimension values.
153-153: No issues found—project supports C++20.The codebase is configured to use C++20 across all build targets (via
cxx_std_20in CMakeLists.txt files). Bothstd::string::ends_with()andstd::ranges::max_elementare valid C++20 features and will compile correctly.
146-162: No validation issues found—edge cases are handled correctly.The provided code snippet was incomplete. While lines 146–162 iterate over
operationsandtargetswithout explicit empty checks, the full implementation (lines 165–167, not shown in the review) properly handles the trailing comma:if (!content.empty() && content.back() == ',') { content.pop_back(); }Empty collections are gracefully handled: the loops simply don't execute, producing valid output (e.g.,
name () qubits;). The constructor allows emptyqubitsandoperationsvectors—no assertion prevents this—but the code does not crash or produce malformed output in these cases.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Just had a quick look and this looks pretty solid.
I only have one small comment on the existing changes.
Some more general observations/questions:
Did you purposefully only add this to the C++ parts of the library?
At the very least, I think you need to expose the new OpType enum member to Python (
core/bindings/ir/operations/register_optype.cpp
Lines 27 to 71 in 98c31eb
core/python/mqt/core/ir/operations.pyi
Lines 74 to 355 in 98c31eb
Preferably, one would also expose the new QuantumComputation convenience method (here
core/bindings/ir/register_quantum_computation.cpp
Lines 428 to 432 in 98c31eb
core/python/mqt/core/ir/__init__.pyi
Lines 1888 to 1906 in 98c31eb
Currently, circuits with bridge gates cannot be properly exported to OpenQASM (no definition for the gate is created and/or it is not decomposed into an actual bridge gate sequence of gates; i.e., the resulting QASM file is not valid).
The created QASM files also can't be read into MQT Core because the parser does not know how to handle the new gate.
Similar points hold for the connection from/to Qiskit.
Any circuit containing the newly introduced gate cannot be handled.
I recently introduced a new kind of Standard Gate in #1283.
That PR pretty much outlines the entire spectrum of changes one could wish for when adding a new gate (including changes to the DD and ZX package, which are probably not that necessary here).
I am not saying that we need all of that as part of this PR, but I wanted to make you aware of the limitations of the current scope of the PR.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Ludwig Schmid <117631861+lsschmid@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ir/operations/AodOperation.cpp (1)
39-43: Need to check for indirect references and whether this is part of a public API:Remove unused
SingleOperation::toQASMString()method (lines 39-43).The refactored
dumpOpenQASMnow inlines operation formatting directly, makingtoQASMString()dead code. Ripgrep found zero call sites—only the definition and declaration. The method can be safely removed from both the implementation and header file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
include/mqt-core/ir/operations/OpType.inc(1 hunks)src/ir/operations/AodOperation.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ir/operations/AodOperation.cpp (5)
include/mqt-core/ir/operations/AodOperation.hpp (5)
of(85-88)maybe_unused(69-69)maybe_unused(69-69)maybe_unused(71-71)maybe_unused(71-71)src/ir/operations/StandardOperation.cpp (2)
dumpOpenQASM(276-290)dumpOpenQASM(276-279)src/ir/QuantumComputation.cpp (3)
dumpOpenQASM(634-703)dumpOpenQASM(634-634)targets(1584-1584)src/ir/operations/CompoundOperation.cpp (2)
dumpOpenQASM(191-199)dumpOpenQASM(191-195)src/ir/operations/NonUnitaryOperation.cpp (2)
dumpOpenQASM(90-126)dumpOpenQASM(90-94)
🔇 Additional comments (5)
include/mqt-core/ir/operations/OpType.inc (2)
68-68: LGTM! Binary compatibility preserved as suggested in past review.The Bridge operation is correctly added at index 42 without shifting any existing operation indices, which preserves binary compatibility with previous releases as discussed in the past review comments.
73-73: LGTM! Correctly updated to reflect the new highest operation index.The LAST_OP_TYPE value of 43 is correct, as it should be one past the highest operation index (Bridge at 42).
src/ir/operations/AodOperation.cpp (3)
45-52: LGTM!The conversion helper is straightforward and correctly transforms the input vector element by element.
106-114: LGTM!The
getStartsmethod correctly filters operations by dimension and collects start values, mirroring the existinggetEndsimplementation pattern.
116-132: LGTM!Both helper methods are well-implemented:
getDistancescorrectly computes absolute differences per dimensiongetMaxDistanceproperly handles the empty case and uses modern C++20 ranges
|
Yes, the changes were made intentionally in the C++ part only, similar to the (Aod)Move Operations. For the current Hybrid Mapper, there is no functionality planned or existing that would require direct Python access. As the Mapper output cannot be valid QASM anyway (due to AOD Movements), I don't really see any benefit and would even argue that it could result in confusion if it exists, but actually it's not intended to be used. A fully fledged definition of the Bridge gate itself (similar to SWAP) might be valuable also in other cases (although it's no really a popular gate), but is out of scope for this PR I would say. |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Alright, makes sense. Just wanted to make sure that you are aware of the consequences.
The one suggestion by code rabbit seems reasonable. Would be nice if you could quickly apply that diff. Then, this is ready to go.
|
Ah, and I missed one (obvious) thing: Could you please add a changelog entry to the changelog? |
2 participants
Description
This PR add functionality needed for an upcoming version of the hybrid mapper in QMAP.
In particular, it adds new types of operations: the "bridge" gate and the "passby" operation (a kind of bridge but with shuttling).
Checklist: